Integrate the sparse index with 'git apply' and interactive add, checkout, and reset #1914
+167
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
apply-sparse-index
branch
from
3e932c1 to
63caae8
Compare
|
Hey, I've just created my first git PR, are you able to |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> The sparse index helps make some Git commands faster when using
> sparse-checkout in cone mode. However, not all code paths are aware that the
> index can have non-blob entries, so we are careful about rolling this
> feature out gradually. The cost of this rollout is that some commands are
> slower with the sparse index as they need to expand a sparse index into a
> full index in memory, which requires parsing tree objects to construct the
> full path list.
>
> This patch series focuses on the 'git add -p' command, which is slow with
> the sparse index for a couple of reasons, handled in the first two patches:
>
> 1. 'git add -p' uses 'git apply' as a subcommand and 'git apply' needs
> integration with the sparse index. Luckily, we just need to add the repo
> setting and appropriate tests to confirm it behaves as expected.
> 2. The interactive modes of 'git add' ('-p' and '-i') leave cmd_add()
> before the code that sets the repo setting to allow for a sparse index.
> Patch 2 fixes this and adds appropriate tests to confirm the behavior in
> a sparse-checkout.
>
> A third patch adds a performance test to p2000-sparse-operations.sh to
> confirm that we are getting the performance improvement we expect:
>
> BASE PATCH 1 PATCH 2
> ---------------------------------------------------------
> 2000.118: (full-v3) 0.80 0.84 +5.0% 0.84 +5.0%
> 2000.119: (full-v4) 0.76 0.79 +3.9% 0.80 +5.3%
> 2000.120: (sparse-v3) 2.09 1.39 -33.5% 0.07 -96.7%
> 2000.121: (sparse-v4) 2.09 1.39 -33.5% 0.07 -96.7%
>
>
> Thanks, -Stolee
As always, it is delight to read a well-written cover letter that
naturally convinces readers why the series is worth reading ;-)
Thanks. |
|
This patch series was integrated into seen via git@9b8a941. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@a4fcd84. |
|
This patch series was integrated into next via git@11ce430. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -12,7 +12,7 @@ static const char * const apply_usage[] = { | |||
| int cmd_apply(int argc, | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> The sparse index allows storing directory entries in the index, marked
> with the skip-wortkree bit and pointing to a tree object. This may be an
> unexpected data shape for some implementation areas, so we are rolling
> it out incrementally on a builtin-per-builtin basis.
>
> This change enables the sparse index for 'git apply'. The main
> motivation for this change is that 'git apply' is used as a child
> process of 'git add -p' and expanding the sparse index for each of those
> child processes can lead to significant performance issues.
>
> The good news is that the actual index manipulation code used by 'git
> apply' is already integrated with the sparse index, so the only product
> change is to mark the builtin as allowing the sparse index so it isn't
> inflated on read.
>
> The more involved part of this change is around adding tests that verify
> how 'git apply' behaves in a sparse-checkout environment and whether or
> not the index expands in certain operations.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> builtin/apply.c | 7 +++-
> t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84f1863d3ac3..a1e20c593d09 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -12,7 +12,7 @@ static const char * const apply_usage[] = {
> int cmd_apply(int argc,
> const char **argv,
> const char *prefix,
> - struct repository *repo UNUSED)
> + struct repository *repo)
> {
> int force_apply = 0;
> int options = 0;
> @@ -35,6 +35,11 @@ int cmd_apply(int argc,
> &state, &force_apply, &options,
> apply_usage);
>
> + if (repo) {
> + prepare_repo_settings(repo);
> + repo->settings.command_requires_full_index = 0;
> + }
> +
> if (check_apply_state(&state, force_apply))
> exit(128);
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index f9b448792cb4..ab8bd371eff3 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1340,6 +1340,30 @@ test_expect_success 'submodule handling' '
> grep "160000 $(git -C initial-repo rev-parse HEAD) 0 modules/sub" cache
> '
>
> +test_expect_success 'git apply functionality' '
> + init_repos &&
> +
> + test_all_match git checkout base &&
> +
> + git -C full-checkout diff base..merge-right -- deep >patch-in-sparse &&
> + git -C full-checkout diff base..merge-right -- folder2 >patch-outside &&
> +
> + # Apply a patch to a file inside the sparse definition
> + test_all_match git apply --index --stat ../patch-in-sparse &&
> + test_all_match git status --porcelain=v2 &&
> +
> + # Apply a patch to a file outside the sparse definition
> + test_sparse_match test_must_fail git apply ../patch-outside &&
> + grep "No such file or directory" sparse-checkout-err &&
I was slightly confused by this at first, because I was thinking of
the case where folder2/a exists in the working directory despite not
matching the sparsity patterns, but you were testing a case where the
working directory matched the sparsity patterns, so folder2/a doesn't
exist.
So, the check here looks good.
And, when folder2/a does exist, then we're in the case handled by
82386b44963f (Merge branch 'en/present-despite-skipped', 2022-03-09),
which forces the directory to not be considered sparse, and so it's
just like the ../patch-in-sparse case...meaning it's not really a
different case to test.
So, it all makes sense.
> +
> + # But it works with --index and --cached
> + test_all_match git apply --index --stat ../patch-outside &&
> + test_all_match git status --porcelain=v2 &&
> + test_all_match git reset --hard &&
> + test_all_match git apply --cached --stat ../patch-outside &&
> + test_all_match git status --porcelain=v2
> +'
> +
> # When working with a sparse index, some commands will need to expand the
> # index to operate properly. If those commands also write the index back
> # to disk, they need to convert the index to sparse before writing.
> @@ -2345,6 +2369,28 @@ test_expect_success 'sparse-index is not expanded: check-attr' '
> ensure_not_expanded check-attr -a --cached -- folder1/a
> '
>
> +test_expect_success 'sparse-index is not expanded: git apply' '
> + init_repos &&
> +
> + git -C sparse-index checkout base &&
> + git -C full-checkout diff base..merge-right -- deep >patch-in-sparse &&
> + git -C full-checkout diff base..merge-right -- folder2 >patch-outside &&
> +
> + # Apply a patch to a file inside the sparse definition
> + ensure_not_expanded apply --index --stat ../patch-in-sparse &&
> +
> + # Apply a patch to a file outside the sparse definition
> + # Fails when caring about the worktree.
> + ensure_not_expanded ! apply ../patch-outside &&
> +
> + # Expands when using --index.
> + ensure_expanded apply --index ../patch-outside &&
> + git -C sparse-index reset --hard &&
All makes sense up to here.
> +
> + # Does not expand when using --cached.
> + ensure_not_expanded apply --cached ../patch-outside
Wait, what? That makes no sense.
After some digging, I see why the test passed, but it's very
misleading. Just before this command, if you ran the following
commands from the sparse-index directory, you'd see the following:
$ rm testme
$ GIT_TRACE2_EVENT=$(pwd)/testme git apply --cached ../patch-outside
$ grep ensure_full_index testme
$
Which matches what you were testing and shows why it passed for you.
But I'd argue the test is not correct and confusing for anyone that
reads it, because:
$ git ls-files -s --sparse | grep folder2
040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
100644 78981922613b2afb6025042ff6bd878ac1994e85 0 folder2/a
In other words, the index was *already* (partially) expanded by the
`git apply --index`, and the `git reset --hard` did not fix that
contrary to expectations. Continuing from here we see:
$ git reset --hard
HEAD is now at 703fd3e initial commit
$ git ls-files -s --sparse | grep folder2
040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
100644 78981922613b2afb6025042ff6bd878ac1994e85 0 folder2/a
$ git sparse-checkout reapply
$ git ls-files -s --sparse | grep folder2
040000 123706f6fc38949628eaf0483edbf97ba21123ae 0 folder2/
So, we need to do a `git sparse-checkout reapply` to make sure we were
actually in the expected fully sparse state. From here...
$ rm testme
$ GIT_TRACE2_EVENT=$(pwd)/testme git apply --cached ../patch-outside
$ grep ensure_full_index testme
{"event":"region_enter","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.856008Z","file":"sparse-index.c","line":372,"repo":1,"nesting":1,"category":"index","label":"ensure_full_index"}
{"event":"region_leave","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.856454Z","file":"sparse-index.c","line":455,"repo":1,"t_rel":0.000446,"nesting":1,"category":"index","label":"ensure_full_index"}
{"event":"region_enter","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.857016Z","file":"sparse-index.c","line":372,"repo":1,"nesting":1,"category":"index","label":"ensure_full_index"}
{"event":"region_leave","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.857135Z","file":"sparse-index.c","line":455,"repo":1,"t_rel":0.000119,"nesting":1,"category":"index","label":"ensure_full_index"}
So, indeed, `git apply --cached ../patch-outside` DOES expand the
index, as I expected. It has to when folder2/ is a directory in the
index, so that we can get a folder2/a entry that we can modify. And
that's just what we see:
$ git ls-files -s --sparse | grep folder2
040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
100644 50f4ca4e6265a0497ec2ee6782648138914ad398 0 folder2/a
Can you add a `git sparse-checkout reapply` right after your `git
reset --hard`, and then switch the ensure_not_expanded to
ensure_expanded for the apply --cached call?There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 5/9/25 11:18 PM, Elijah Newren wrote:
> On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
>> + # Expands when using --index.
>> + ensure_expanded apply --index ../patch-outside &&
>> + git -C sparse-index reset --hard &&
> > All makes sense up to here.
> >> +
>> + # Does not expand when using --cached.
>> + ensure_not_expanded apply --cached ../patch-outside
> > Wait, what? That makes no sense.
> > After some digging, I see why the test passed, but it's very
> misleading. Just before this command, if you ran the following
> commands from the sparse-index directory, you'd see the following:
> > $ rm testme
> $ GIT_TRACE2_EVENT=$(pwd)/testme git apply --cached ../patch-outside
> $ grep ensure_full_index testme
> $
> > Which matches what you were testing and shows why it passed for you.
> But I'd argue the test is not correct and confusing for anyone that
> reads it, because:
> > $ git ls-files -s --sparse | grep folder2
> 040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
> 100644 78981922613b2afb6025042ff6bd878ac1994e85 0 folder2/a
> > In other words, the index was *already* (partially) expanded by the
> `git apply --index`, and the `git reset --hard` did not fix that
> contrary to expectations. Continuing from here we see:
> > $ git reset --hard
> HEAD is now at 703fd3e initial commit
> $ git ls-files -s --sparse | grep folder2
> 040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
> 100644 78981922613b2afb6025042ff6bd878ac1994e85 0 folder2/a
> $ git sparse-checkout reapply
> $ git ls-files -s --sparse | grep folder2
> 040000 123706f6fc38949628eaf0483edbf97ba21123ae 0 folder2/
> > So, we need to do a `git sparse-checkout reapply` to make sure we were
> actually in the expected fully sparse state. From here...
> > $ rm testme
> $ GIT_TRACE2_EVENT=$(pwd)/testme git apply --cached ../patch-outside
> $ grep ensure_full_index testme
> {"event":"region_enter","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.856008Z","file":"sparse-index.c","line":372,"repo":1,"nesting":1,"category":"index","label":"ensure_full_index"}
> {"event":"region_leave","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.856454Z","file":"sparse-index.c","line":455,"repo":1,"t_rel":0.000446,"nesting":1,"category":"index","label":"ensure_full_index"}
> {"event":"region_enter","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.857016Z","file":"sparse-index.c","line":372,"repo":1,"nesting":1,"category":"index","label":"ensure_full_index"}
> {"event":"region_leave","sid":"20250510T030856.854763Z-H8ec63c79-P0000371c","thread":"main","time":"2025-05-10T03:08:56.857135Z","file":"sparse-index.c","line":455,"repo":1,"t_rel":0.000119,"nesting":1,"category":"index","label":"ensure_full_index"}
> > So, indeed, `git apply --cached ../patch-outside` DOES expand the
> index, as I expected. It has to when folder2/ is a directory in the
> index, so that we can get a folder2/a entry that we can modify. And
> that's just what we see:
> > $ git ls-files -s --sparse | grep folder2
> 040000 cb4007891397aa2a451037d1c69e57f0cf498c24 0 folder2/0/
> 100644 50f4ca4e6265a0497ec2ee6782648138914ad398 0 folder2/a
> > > Can you add a `git sparse-checkout reapply` right after your `git
> reset --hard`, and then switch the ensure_not_expanded to
> ensure_expanded for the apply --cached call?
Thanks for digging in here. I'm going to augment the test to
include both the ensure_expanded and ensure_not_expanded so we
get the extra coverage for these two scenarios.
Thanks for the careful eye!
-Stolee
| @@ -390,6 +390,10 @@ int cmd_add(int argc, | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> It is slow to expand a sparse index in-memory due to parsing of trees.
> We aim to minimize that performance cost when possible. 'git add -p'
> uses 'git apply' child processes to modify the index, but still there
> are some expansions that occur.
still there are some expansions that occur...outside of those child
processes? Is that what you're trying to say, or was it something
else?
> It turns out that control flows out of cmd_add() in the interactive
> cases before the lines that confirm that the builtin is integrated with
> the sparse index. We need to move that earlier to ensure it prevents a
> full index expansion on read.
>
> Add more test cases that confirm that these interactive add options work
> with the sparse index. One interesting aspect here is that the '-i'
> option avoids expanding the sparse index when a sparse directory exists
> on disk while the '-p' option does hit the ensure_full_index() method.
> This leaves some room for improvement, but this case should be atypical
> as users should remain within their sparse-checkout.
It's not clear whether this paragraph is talking about existing state
(before the patch) or desired state (after the patch). I think based
on the context it's the former, but the last sentence sounds more like
a future work direction that makes it very unclear, to me at least.
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> builtin/add.c | 7 +--
> t/t1092-sparse-checkout-compatibility.sh | 56 ++++++++++++++++++++++++
> 2 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 747511b68bc3..7c292ffdc6c2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -390,6 +390,10 @@ int cmd_add(int argc,
>
> argc = parse_options(argc, argv, prefix, builtin_add_options,
> builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
> +
> + prepare_repo_settings(repo);
> + repo->settings.command_requires_full_index = 0;
> +
> if (patch_interactive)
> add_interactive = 1;
> if (add_interactive) {
> @@ -426,9 +430,6 @@ int cmd_add(int argc,
> add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;
> require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
>
> - prepare_repo_settings(repo);
> - repo->settings.command_requires_full_index = 0;
> -
> repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
>
> /*
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index ab8bd371eff3..0dc5dd27184d 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -384,6 +384,38 @@ test_expect_success 'add, commit, checkout' '
> test_all_match git checkout -
> '
>
> +test_expect_success 'git add -p' '
> + init_repos &&
> +
> + write_script edit-contents <<-\EOF &&
> + echo text >>$1
> + EOF
> +
> + # Does not expand when edits are within sparse checkout.
> + run_on_all ../edit-contents deep/a &&
> + run_on_all ../edit-contents deep/deeper1/a &&
> +
> + test_write_lines y n >in &&
> + run_on_all git add -p <in &&
> + test_all_match git status --porcelain=v2 &&
> + test_all_match git reset &&
> +
> + test_write_lines u 1 "" q >in &&
> + run_on_all git add -i <in &&
> + test_all_match git status --porcelain=v2 &&
> + test_all_match git reset --hard &&
> +
> + run_on_sparse mkdir -p folder1 &&
> + run_on_all ../edit-contents folder1/a &&
> + test_write_lines y n y >in &&
> + run_on_all git add -p <in &&
> + test_sparse_match git status --porcelain=v2 &&
> + test_sparse_match git reset &&
> + test_write_lines u 2 3 "" q >in &&
> + run_on_all git add -i <in &&
> + test_sparse_match git status --porcelain=v2
> +'
> +
> test_expect_success 'deep changes during checkout' '
> init_repos &&
>
> @@ -2391,6 +2423,30 @@ test_expect_success 'sparse-index is not expanded: git apply' '
> ensure_not_expanded apply --cached ../patch-outside
> '
>
> +test_expect_success 'sparse-index is not expanded: git add -p' '
> + init_repos &&
> +
> + # Does not expand when edits are within sparse checkout.
> + echo "new content" >sparse-index/deep/a &&
> + echo "new content" >sparse-index/deep/deeper1/a &&
> + test_write_lines y n >in &&
> + ensure_not_expanded add -p <in &&
> + git -C sparse-index reset &&
> + ensure_not_expanded add -i <in &&
> +
> + mkdir -p sparse-index/folder1 &&
> + echo "new content" >sparse-index/folder1/a &&
> +
> + # -p does expand when edits are outside sparse checkout.
> + test_write_lines y n y >in &&
> + ensure_expanded add -p <in &&
> +
> + # but -i does not expand.
> + git -C sparse-index reset &&
> + test_write_lines u 2 3 "" q >in &&
> + ensure_not_expanded add -i <in
This has the same error as patch 1, in that you assume your reset
above (which wasn't even a reset --hard) will re-sparsify the index.
Since it doesn't, your test is misleading and only shows that when
already expanded to include the files of interest it doesn't expand
any further. To re-sparsify your index before the `add -i` call,
you'll need to do a `git reset --hard && git sparse-checkout reapply`
and then recreate folder1/a with "new content" again...and then run
your 'add -i' command.
Anyway, now I think I understand the expected meaning of the final
paragraph of your commit message, but you were tripped up by assuming
the index was re-sparsified between your steps when it wasn't.There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 5/10/25 12:38 AM, Elijah Newren wrote:
> On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
> <[email protected]> wrote:
>>
>> From: Derrick Stolee <[email protected]>
>>
>> It is slow to expand a sparse index in-memory due to parsing of trees.
>> We aim to minimize that performance cost when possible. 'git add -p'
>> uses 'git apply' child processes to modify the index, but still there
>> are some expansions that occur.
> > still there are some expansions that occur...outside of those child
> processes? Is that what you're trying to say, or was it something
> else?
> >> It turns out that control flows out of cmd_add() in the interactive
>> cases before the lines that confirm that the builtin is integrated with
>> the sparse index. We need to move that earlier to ensure it prevents a
>> full index expansion on read.
>>
>> Add more test cases that confirm that these interactive add options work
>> with the sparse index. One interesting aspect here is that the '-i'
>> option avoids expanding the sparse index when a sparse directory exists
>> on disk while the '-p' option does hit the ensure_full_index() method.
>> This leaves some room for improvement, but this case should be atypical
>> as users should remain within their sparse-checkout.
> > It's not clear whether this paragraph is talking about existing state
> (before the patch) or desired state (after the patch). I think based
> on the context it's the former, but the last sentence sounds more like
> a future work direction that makes it very unclear, to me at least.
I'll try to rewrite to make this clearer.
>> + # -p does expand when edits are outside sparse checkout.
>> + test_write_lines y n y >in &&
>> + ensure_expanded add -p <in &&
>> +
>> + # but -i does not expand.
>> + git -C sparse-index reset &&
>> + test_write_lines u 2 3 "" q >in &&
>> + ensure_not_expanded add -i <in
> > This has the same error as patch 1, in that you assume your reset
> above (which wasn't even a reset --hard) will re-sparsify the index.
> Since it doesn't, your test is misleading and only shows that when
> already expanded to include the files of interest it doesn't expand
> any further. To re-sparsify your index before the `add -i` call,
> you'll need to do a `git reset --hard && git sparse-checkout reapply`
> and then recreate folder1/a with "new content" again...and then run
> your 'add -i' command.
Thanks. I didn't like that this was different. I appreciate your
expertise helping to clarify this issue.
Thanks,
-Stolee
| @@ -135,5 +135,6 @@ test_perf_on_all git diff-tree HEAD | |||
| test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, May 6, 2025 at 5:55 PM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> The previous two changes contributed performance improvements to 'git
> apply' and 'git add -p' when using a sparse index. Add a performance
> test to demonstrate this (and to help validate that performance remains
> good in the future).
>
> In the truncated test output below, we see that the full checkout
> performance changes within noise expectations, but the sparse index
> cases improve 33% and then 96%.
>
> HEAD~3 HEAD~2 HEAD~1
> ---------------------------------------------------------
> 2000.118: (full-v3) 0.80 0.84 +5.0% 0.84 +5.0%
> 2000.119: (full-v4) 0.76 0.79 +3.9% 0.80 +5.3%
> 2000.120: (sparse-v3) 2.09 1.39 -33.5% 0.07 -96.7%
> 2000.121: (sparse-v4) 2.09 1.39 -33.5% 0.07 -96.7%
>
> It is worth noting that if our test was more involved and had multiple
> hunks to evaluate, then the time spent in 'git apply' would dominate due
> to multiple index loads and writes. As it stands, we need the sparse
> index improvement in 'git add -p' itself to confirm this performance
> improvement.
>
> Since the change for 'git add -i' is identical, we avoid a second test
> case for that similar operation.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> t/perf/p2000-sparse-operations.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 39e92b084143..3da6ae59c000 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -135,5 +135,6 @@ test_perf_on_all git diff-tree HEAD
> test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a
> test_perf_on_all "git worktree add ../temp && git worktree remove ../temp"
> test_perf_on_all git check-attr -a -- $SPARSE_CONE/a
> +test_perf_on_all 'echo >>a && test_write_lines y | git add -p'
>
> test_done
> --
> gitgitgadget
Very nice!
|
This patch series was integrated into seen via git@aab549b. |
|
There was a status update in the "New Topics" section about the branch source: <[email protected]> |
|
This patch series was integrated into seen via git@86caedb. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Stolee
On 07/05/2025 01:55, Derrick Stolee via GitGitGadget wrote:
> The sparse index helps make some Git commands faster when using
> sparse-checkout in cone mode. However, not all code paths are aware that the
> index can have non-blob entries, so we are careful about rolling this
> feature out gradually. The cost of this rollout is that some commands are
> slower with the sparse index as they need to expand a sparse index into a
> full index in memory, which requires parsing tree objects to construct the
> full path list.
> > This patch series focuses on the 'git add -p' command, which is slow with
> the sparse index for a couple of reasons, handled in the first two patches:
> > 1. 'git add -p' uses 'git apply' as a subcommand and 'git apply' needs
> integration with the sparse index. Luckily, we just need to add the repo
> setting and appropriate tests to confirm it behaves as expected.
> 2. The interactive modes of 'git add' ('-p' and '-i') leave cmd_add()
> before the code that sets the repo setting to allow for a sparse index.
> Patch 2 fixes this and adds appropriate tests to confirm the behavior in
> a sparse-checkout.
This made me wonder about the other commands that take "--patch" like checkout and reset. Do you know how well they handle the sparse index? They'll all benefit from the changes to git apply in this series but I was wondering if they need any further changes.
Best Wishes
Phillip
> A third patch adds a performance test to p2000-sparse-operations.sh to
> confirm that we are getting the performance improvement we expect:
> > BASE PATCH 1 PATCH 2
> ---------------------------------------------------------
> 2000.118: (full-v3) 0.80 0.84 +5.0% 0.84 +5.0%
> 2000.119: (full-v4) 0.76 0.79 +3.9% 0.80 +5.3%
> 2000.120: (sparse-v3) 2.09 1.39 -33.5% 0.07 -96.7%
> 2000.121: (sparse-v4) 2.09 1.39 -33.5% 0.07 -96.7%
> > > Thanks, -Stolee
> > Derrick Stolee (3):
> apply: integrate with the sparse index
> git add: make -p/-i aware of sparse index
> p2000: add performance test for 'git add -p'
> > builtin/add.c | 7 +-
> builtin/apply.c | 7 +-
> t/perf/p2000-sparse-operations.sh | 1 +
> t/t1092-sparse-checkout-compatibility.sh | 102 +++++++++++++++++++++++
> 4 files changed, 113 insertions(+), 4 deletions(-)
> > > base-commit: 6c0bd1fc70efaf053abe4e57c976afdc72d15377
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1914%2Fderrickstolee%2Fapply-sparse-index-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1914/derrickstolee/apply-sparse-index-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1914
|
|
User |
|
This patch series was integrated into seen via git@ac167a0. |
|
This patch series was integrated into seen via git@3eb1552. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/14/25 11:16 AM, Phillip Wood wrote:
> This made me wonder about the other commands that take "--patch" like checkout > and reset. Do you know how well they handle the sparse index? They'll all > benefit from the changes to git apply in this series but I was wondering if they > need any further changes.
By adding these two tests...
test_perf_on_all 'test_write_lines y y y | git checkout --patch -'
test_perf_on_all 'echo >>a && git add a && test_write_lines y | git reset --patch'
...we can demonstrate that the changes to 'git apply' are sufficient
to get the improvements we seek for 'git checkout --patch':
Test HEAD~3 HEAD~2
---------------------------------------------------------------
... git checkout --patch - (full-v3) 1.22 1.22 +0.0%
... git checkout --patch - (full-v4) 1.15 1.16 +0.9%
... git checkout --patch - (sparse-v3) 1.37 0.11 -92.0%
... git checkout --patch - (sparse-v4) 1.37 0.11 -92.0%
... git reset --patch (full-v3) 0.82 0.81 -1.2%
... git reset --patch (full-v4) 0.76 0.77 +1.3%
... git reset --patch (sparse-v3) 1.57 0.91 -42.0%
... git reset --patch (sparse-v4) 1.59 0.92 -42.1%
But 'git reset --patch' appears to not be fast _enough_. It turns
out that it has the same issue as cmd_add(). I'll add a patch for
this purpose.
Thanks,
-Stolee
|
derrickstolee
force-pushed
the
apply-sparse-index
branch
from
7a77728 to
a161be2
Compare
The sparse index allows storing directory entries in the index, marked with the skip-wortkree bit and pointing to a tree object. This may be an unexpected data shape for some implementation areas, so we are rolling it out incrementally on a builtin-per-builtin basis. This change enables the sparse index for 'git apply'. The main motivation for this change is that 'git apply' is used as a child process of 'git add -p' and expanding the sparse index for each of those child processes can lead to significant performance issues. The good news is that the actual index manipulation code used by 'git apply' is already integrated with the sparse index, so the only product change is to mark the builtin as allowing the sparse index so it isn't inflated on read. The more involved part of this change is around adding tests that verify how 'git apply' behaves in a sparse-checkout environment and whether or not the index expands in certain operations. Signed-off-by: Derrick Stolee <[email protected]>
It is slow to expand a sparse index in-memory due to parsing of trees. We aim to minimize that performance cost when possible. 'git add -p' uses 'git apply' child processes to modify the index, but still there are some expansions that occur. It turns out that control flows out of cmd_add() in the interactive cases before the lines that confirm that the builtin is integrated with the sparse index. Moving that integration point earlier in cmd_add() allows 'git add -p' and 'git add -p' to operate without expanding a sparse index to a full one. Add test cases that confirm that these interactive add options work with the sparse index. Signed-off-by: Derrick Stolee <[email protected]>
Similar to the previous change for 'git add -p', the reset builtin checked for integration with the sparse index after possibly redirecting its logic toward the interactive logic. This means that the builtin would expand the sparse index to a full one upon read. Move this check earlier within cmd_reset() to improve performance here. Add tests to guarantee that we are not universally expanding the index. Add behavior tests to check that we are doing the same operations as a full index. Signed-off-by: Derrick Stolee <[email protected]>
The previous three changes contributed performance improvements to 'git apply', 'git add -p', and 'git reset -p' when using a sparse index. The improvement to 'git apply' also improved 'git checkout -p'. Add performance tests to demonstrate this (and to help validate that performance remains good in the future). In the truncated test output below, we see that the full checkout performance changes within noise expectations, but the sparse index cases improve 33% and then 96% for 'git add -p' and 41% and then 95% for 'git reset -p'. 'git checkout -p' improves immediatley by 91% because it does not need any change to its builtin. Test HEAD~4 HEAD~3 HEAD~2 HEAD~1 ------------------------------------------------------------------------------------- 2000.118: ... git add -p (full-v3) 0.79 0.79 +0.0% 0.82 +3.8% 0.82 +3.8% 2000.119: ... git add -p (full-v4) 0.74 0.76 +2.7% 0.74 +0.0% 0.76 +2.7% 2000.120: ... git add -p (sparse-v3) 1.94 1.28 -34.0% 0.07 -96.4% 0.07 -96.4% 2000.121: ... git add -p (sparse-v4) 1.93 1.28 -33.7% 0.06 -96.9% 0.06 -96.9% 2000.122: ... git checkout -p (full-v3) 1.18 1.18 +0.0% 1.18 +0.0% 1.19 +0.8% 2000.123: ... git checkout -p (full-v4) 1.10 1.12 +1.8% 1.11 +0.9% 1.11 +0.9% 2000.124: ... git checkout -p (sparse-v3) 1.31 0.11 -91.6% 0.11 -91.6% 0.11 -91.6% 2000.125: ... git checkout -p (sparse-v4) 1.29 0.11 -91.5% 0.11 -91.5% 0.11 -91.5% 2000.126: ... git reset -p (full-v3) 0.81 0.80 -1.2% 0.83 +2.5% 0.83 +2.5% 2000.127: ... git reset -p (full-v4) 0.78 0.77 -1.3% 0.77 -1.3% 0.78 +0.0% 2000.128: ... git reset -p (sparse-v3) 1.58 0.92 -41.8% 0.91 -42.4% 0.07 -95.6% 2000.129: ... git reset -p (sparse-v4) 1.58 0.92 -41.8% 0.92 -41.8% 0.07 -95.6% It is worth noting that if our test was more involved and had multiple hunks to evaluate, then the time spent in 'git apply' would dominate due to multiple index loads and writes. As it stands, we need the sparse index improvement in 'git add -p' itself to confirm this performance improvement. Since the change for 'git add -i' is identical, we avoid a second test case for that similar operation. Signed-off-by: Derrick Stolee <[email protected]>
derrickstolee
force-pushed
the
apply-sparse-index
branch
from
a161be2 to
a50c57f
Compare
derrickstolee
changed the title
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Fri, May 16, 2025 at 7:55 AM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
[...]
> Updates in v2
> =============
>
> Thanks for the careful review from Elijah and the pointer from Phillip, we
> have these changes:
>
> 1. The tests no longer have different expansion behaviors for 'git add -p'
> and 'git add -i' due to partially-expanded indexes on disk.
> 2. We now test 'git checkout -p' and 'git reset -p'.
> 3. 'git reset -p' needed some changes to the builtin (similar to 'git add')
> to be fast.
I haven't looked at the new patch yet, but the updated patches 1 & 2
look much improved (though there's still one problem with the new
commit message, which I comment on in the range-diff below).
However, I think Junio already merged your v1 to next
(https://lore.kernel.org/git/CABPp-BEukTWwsuC7MMR8D5_UAhyw-LgT=[email protected]/).
So he'll either have to revert your v1 in next and apply the new
series on top, or you'll need to re-roll as fixes on top of your v1.
> 2: 63caae87634 ! 2: 0a2752721d0 git add: make -p/-i aware of sparse index
> @@ Commit message
>
> It turns out that control flows out of cmd_add() in the interactive
> cases before the lines that confirm that the builtin is integrated with
> - the sparse index. We need to move that earlier to ensure it prevents a
> - full index expansion on read.
> + the sparse index.
>
> - Add more test cases that confirm that these interactive add options work
> - with the sparse index. One interesting aspect here is that the '-i'
> - option avoids expanding the sparse index when a sparse directory exists
> - on disk while the '-p' option does hit the ensure_full_index() method.
> - This leaves some room for improvement, but this case should be atypical
> - as users should remain within their sparse-checkout.
> + Moving that integration point earlier in cmd_add() allows 'git add -p'
> + and 'git add -p' to operate without expanding a sparse index to a full
> + one.
Was the second 'git add -p' meant to be 'git add -i'?
> -: ----------- > 3: d1482a29d8f reset: integrate sparse index with --patch
Other than the one comment above, your changes from the range-diff
look good to me for patches 1 & 2, and the new 4. I haven't looked at
this new patch 3 yet but wanted to comment on the merged-to-next
issue. |
| @@ -420,6 +420,9 @@ int cmd_reset(int argc, | |||
| oidcpy(&oid, &tree->object.oid); | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Fri, May 16, 2025 at 7:55 AM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> Similar to the previous change for 'git add -p', the reset builtin
> checked for integration with the sparse index after possibly redirecting
> its logic toward the interactive logic. This means that the builtin
> would expand the sparse index to a full one upon read.
>
> Move this check earlier within cmd_reset() to improve performance here.
>
> Add tests to guarantee that we are not universally expanding the index.
> Add behavior tests to check that we are doing the same operations as a
> full index.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> builtin/reset.c | 6 ++--
> t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++--
> 2 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 73b4537a9a56..dc50ffc1ac59 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -420,6 +420,9 @@ int cmd_reset(int argc,
> oidcpy(&oid, &tree->object.oid);
> }
>
> + prepare_repo_settings(the_repository);
> + the_repository->settings.command_requires_full_index = 0;
> +
> if (patch_mode) {
> if (reset_type != NONE)
> die(_("options '%s' and '%s' cannot be used together"), "--patch", "--{hard,mixed,soft}");
> @@ -457,9 +460,6 @@ int cmd_reset(int argc,
> if (intent_to_add && reset_type != MIXED)
> die(_("the option '%s' requires '%s'"), "-N", "--mixed");
>
> - prepare_repo_settings(the_repository);
> - the_repository->settings.command_requires_full_index = 0;
> -
> if (repo_read_index(the_repository) < 0)
> die(_("index file corrupt"));
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index c419d8b57e84..d8101139b40a 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -384,7 +384,7 @@ test_expect_success 'add, commit, checkout' '
> test_all_match git checkout -
> '
>
> -test_expect_success 'git add -p' '
> +test_expect_success 'git add, checkout, and reset with -p' '
> init_repos &&
>
> write_script edit-contents <<-\EOF &&
> @@ -398,7 +398,7 @@ test_expect_success 'git add -p' '
> test_write_lines y n >in &&
> run_on_all git add -p <in &&
> test_all_match git status --porcelain=v2 &&
> - test_all_match git reset &&
> + test_all_match git reset -p <in &&
>
> test_write_lines u 1 "" q >in &&
> run_on_all git add -i <in &&
> @@ -413,6 +413,12 @@ test_expect_success 'git add -p' '
> test_sparse_match git reset &&
> test_write_lines u 2 3 "" q >in &&
> run_on_all git add -i <in &&
> + test_sparse_match git status --porcelain=v2 &&
> +
> + run_on_all git add --sparse folder1 &&
> + run_on_all git commit -m "take changes" &&
> + test_write_lines y n y >in &&
> + test_sparse_match git checkout HEAD~1 --patch <in &&
> test_sparse_match git status --porcelain=v2
> '
>
> @@ -2458,6 +2464,38 @@ test_expect_success 'sparse-index is not expanded: git add -p' '
> ensure_expanded add -i <in
> '
>
> +test_expect_success 'sparse-index is not expanded: checkout -p, reset -p' '
> + init_repos &&
> +
> + # Does not expand when edits are within sparse checkout.
> + echo "new content" >sparse-index/deep/a &&
> + echo "new content" >sparse-index/deep/deeper1/a &&
> + git -C sparse-index commit -a -m "inside-changes" &&
> +
> + test_write_lines y y >in &&
> + ensure_not_expanded checkout HEAD~1 --patch <in &&
> +
> + echo "new content" >sparse-index/deep/a &&
> + echo "new content" >sparse-index/deep/deeper1/a &&
> + git -C sparse-index add . &&
> + ensure_not_expanded reset --patch <in &&
> +
> + # -p does expand when edits are outside sparse checkout.
> + mkdir -p sparse-index/folder1 &&
> + echo "new content" >sparse-index/folder1/a &&
> + git -C sparse-index add --sparse folder1 &&
> + git -C sparse-index sparse-checkout reapply &&
> + ensure_expanded reset --patch <in &&
> +
> + # Fully reset the index.
> + mkdir -p sparse-index/folder1 &&
> + echo "new content" >sparse-index/folder1/a &&
> + git -C sparse-index add --sparse folder1 &&
> + git -C sparse-index commit -m "folder1 change" &&
> + git -C sparse-index sparse-checkout reapply &&
> + ensure_expanded checkout HEAD~1 --patch <in
> +'
> +
> test_expect_success 'advice.sparseIndexExpanded' '
> init_repos &&
>
> --
> gitgitgadget
Patch looks good to me.
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 5/16/2025 11:32 AM, Elijah Newren wrote:
> On Fri, May 16, 2025 at 7:55 AM Derrick Stolee via GitGitGadget
> <[email protected]> wrote:
>>
>> - Add more test cases that confirm that these interactive add options work
>> - with the sparse index. One interesting aspect here is that the '-i'
>> - option avoids expanding the sparse index when a sparse directory exists
>> - on disk while the '-p' option does hit the ensure_full_index() method.
>> - This leaves some room for improvement, but this case should be atypical
>> - as users should remain within their sparse-checkout.
>> + Moving that integration point earlier in cmd_add() allows 'git add -p'
>> + and 'git add -p' to operate without expanding a sparse index to a full
>> + one.
>
> Was the second 'git add -p' meant to be 'git add -i'?
You are right. The second one should be 'git add -i'.
And regarding 'next', I do expect my v1 to be ejected from 'next' while
review continues on this v2. If instead it gets merged to 'master', then
I'll prepare a new series on top that applies the learnings from this
review.
Thanks,
-Stolee
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The sparse index helps make some Git commands faster when using sparse-checkout in cone mode. However, not all code paths are aware that the index can have non-blob entries, so we are careful about rolling this feature out gradually. The cost of this rollout is that some commands are slower with the sparse index as they need to expand a sparse index into a full index in memory, which requires parsing tree objects to construct the full path list.
This patch series focuses on the 'git add -p' command, which is slow with the sparse index for a couple of reasons, handled in the first two patches:
A third patch adds a performance test to p2000-sparse-operations.sh to confirm that we are getting the performance improvement we expect:
Updates in v2
Thanks for the careful review from Elijah and the pointer from Phillip, we have these changes:
Thanks,
-Stolee
cc: [email protected]
cc: [email protected]
cc: Phillip Wood [email protected]